ResultBuilders use vars instead of lets internally, leading to unexpected parameter packing
#85925
+37
−1
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It appears that using
ResultBuilders to create a parameter-pack type viabuildPartial(first:)/buildPartial(accumulated:next:), the resulting packing is not optimal.For reasons that weren't initialy clear, each round of creating a tuple via a result builder would result in deeper and deeper tuple nesting.
Background
I've detailed my findings in #85924, but I found the solution to this via a chance change to
BuilderTransformwhile doing other work to try to erase the@lValue-ness of parameters created inBuilderTransform::buildCall.After making a chance change ("Why are these
vars and notlets may as well change this..."), the compiler I was building suddenly worked, when all I had added otherwise was logging!In this light, the solution to #85837 is simply this change:
lib/Sema/BuilderTransform.cpp VarDecl *ResultBuilder::buildVar(SourceLoc loc) { auto &ctx = DC->getASTContext(); // Create the implicit variable. Identifier name = ctx.getIdentifier(("$__builder" + Twine(VarCounter++)).str()); auto var = new (ctx) - VarDecl(/*isStatic=*/false, VarDecl::Introducer::Var, loc, name, DC); + VarDecl(/*isStatic=*/false, VarDecl::Introducer::Let, loc, name, DC); var->setImplicit(); return var; }Repro
Given this result builder:
I would expect this code (Example, Constraints):
To be equivalent to (Example, Constraints):
But it is in fact constructed like so:
Issue
I've documented this behavior in #85924 , but when parameter-packed generics are passed a reference to a
varvalue, theIf you continue you end up with tuples packed like:
Change
The change is pretty straight forward, just make all the
VarDecl's intolets:lib/Sema/BuilderTransform.cpp VarDecl *ResultBuilder::buildVar(SourceLoc loc) { auto &ctx = DC->getASTContext(); // Create the implicit variable. Identifier name = ctx.getIdentifier(("$__builder" + Twine(VarCounter++)).str()); auto var = new (ctx) - VarDecl(/*isStatic=*/false, VarDecl::Introducer::Var, loc, name, DC); + VarDecl(/*isStatic=*/false, VarDecl::Introducer::Let, loc, name, DC); var->setImplicit(); return var; }Concerns
This is the way I'd expect this to act, but I'm not sure if everyone agrees or if there's not existing code that might be dependent on this behavior.
I've documented my thoughts in #85837 and in the thread in the Swift forums.
Impact
This changes all result builders to have different semantics internally and in at least this case, the type returned from a builder could be substantively different.
This would allow for maximally-flattened tuples without workarounds, but it might break any clients explicitly depending on this nested-packing behavior.
Altertative/Enhanced Options
Multiple constraints:
Is it possible to have have both options as constraints? As in generate one constraint with
letwhich takes prescidence, but if specifically requested, fall back to thevarequivalent?I'm not sure what the constraint solver overhead for this maneuver is.
Fix #85924 (Parameter packed functions pack less efficiently if the input is a
var):Solving what seems like the root issue would potentially obviate the need for this particular fix. The scope/impact of the change would be greatly increased, though. It seems like it carries considerably more risk to the system.
Apply this change and fix #85924 :
Unless I'm missing something (do...result builders support
inouts?), it seems sub-optimal, or at least unecessary, to have every value generated by a builder be avarand never mutated.If there's a larger fix for #85924 that also fixes this issue, it might still be worth taking this change.
Misc
My first interaction with parameter packs was also me finding this bug, so I'm not entirely sure how they're supposed to work, packing wise (are less optimal packings still valid)?
Mainly
(String, Int, String)seems like it should be the right "choice", but I'm unclear on if((String, Int), String)is also a vailid option.If the latter is a valid option there may be additional bugs (see #85924). There also may be existing code that depends on the
((String, Int), String)-style choice.